Route security management by end user #14536
Conversation
|
@openshift/networking @knobunc @jorgemoralespou PTAL |
|
@fbac Take a look at this PR |
|
Great work, thank guys! 👍 |
| @@ -6,6 +6,9 @@ | |||
| {{- define "/var/lib/haproxy/conf/haproxy.config" }} | |||
| {{- $workingDir := .WorkingDir }} | |||
| {{- $defaultDestinationCA := .DefaultDestinationCA }} | |||
| {{- $cidrPattern := "(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])(/([0-9]|[1-2][0-9]|3[0-2]))?" -}} | |||
There was a problem hiding this comment.
We could make this a hair less ugly by having:
{{- $quadPattern := "(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])" -}}
{{- $ipPattern := printf "(?:%s\.%s\.%s\.%s)" $quadPattern $quadPattern $quadPattern $quadPattern -}}
{{- $cidrPattern := printf "(?:%s(?:/(?:[0-9]|[1-2][0-9]|3[0-2]))?)" $ipPattern -}}
I also think we should wrap all named regexes in ()s (as I did above) so that they are more easily composed into other regexes without the alternations getting messed up. (i.e. if I have $foo := "a|b"; then that is risky to use in a subsequent regex.
Also all regexes should not be using capturing ()s. We should say (?:) instead.
Finally, all regexes should have a comment explaining what they match, with a sample match.
| @@ -6,6 +6,9 @@ | |||
| {{- define "/var/lib/haproxy/conf/haproxy.config" }} | |||
| {{- $workingDir := .WorkingDir }} | |||
| {{- $defaultDestinationCA := .DefaultDestinationCA }} | |||
| {{- $cidrPattern := "(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])(/([0-9]|[1-2][0-9]|3[0-2]))?" -}} | |||
| {{- $cidrListPattern := printf "%s( +(%s))*" $cidrPattern $cidrPattern -}} | |||
There was a problem hiding this comment.
Based on the above rules, this should be:
{{- $cidrListPattern := printf "(?:%s(?: +%s)*)" $cidrPattern $cidrPattern -}}
|
@knobunc is this what you are looking for? PTAL |
|
|
||
| {{/* A bunch of regular expressions. Each should be wrapped in (?:) so that it is safe to include bare */}} | ||
| {{/* quadPattern: Match a quad in an IP address; e.g. 123 */}} | ||
| {{- $quadPattern := "(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])" -}} |
There was a problem hiding this comment.
Much better. But can we flip all of these regexes to be enclosed in backticks, that way we can lose any double \ stuff. Do it to all of the ones you are adding just for consistency.
Add a new route annotation "haproxy.router.openshift.io/ip_whitelist" that specifies a space separated list of white listed source IP addresses and/or CIDRs. Requests from IP addresses that are not in the whitelist are dropped. When the annotation is present for a route a acl is set up in the backend with the whitelist. This PR addresses issue openshift#13709 Some examples: When editing a route add the following annotation to define the desired source ip's. 1) allow only one ip haproxy.router.openshift.io/whitelist: 192.168.1.10 2) several ip's haproxy.router.openshift.io/whitelist: 192.168.1.10 192.168.1.11 192.168.1.12 3) ip ranges haproxy.router.openshift.io/whitelist: 192.168.1.0/24 4) ip's and ranges haproxy.router.openshift.io/whitelist: 180.5.61.153 192.168.1.0/24 10.0.0.0/8 Trello: TbZPhHKE Route security management by end user https://trello.com/c/TbZPhHKE/ Bug: 1426562 https://bugzilla.redhat.com/show_bug.cgi?id=1426562 Committer: [email protected] Author: [email protected]
|
@knobunc I put in the `, is this ready to merge? |
|
@pecameron I'd like someone else to weigh in. I flagged @rajatchopra for review too. |
|
[merge] |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 7c4719f |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2229/) (Base Commit: ee9f436) |
|
[merge][severity: bug] |
|
Evaluated for origin merge up to 7c4719f |
|
continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1010/) (Base Commit: 98b2737) (PR Branch Commit: 7c4719f) (Extended Tests: bug) |
|
False merge failure, merging anyway |
Openshift 3.6 Add a new route annotation "haproxy.router.openshift.io/ip_whitelist" that specifies a space separated list of white listed source IP addresses and/or CIDRs. Requests from IP addresses that are not in the whitelist are dropped. origin PR 14536 openshift/origin#14536 Trello: TbZPhHKE Route security management by end user https://trello.com/c/TbZPhHKE/ Bug: 1426562 https://bugzilla.redhat.com/show_bug.cgi?id=1426562
Add a new route annotation "haproxy.router.openshift.io/ip_whitelist"
that specifies a space separated list of white listed source IP
addresses and/or CIDRs. Requests from IP addresses that are not in
the whitelist are dropped.
When the annotation is present for a route a acl is set up in the
backend with the whitelist.
This PR addresses issue #13709
Some examples:
When editing a route add the following annotation to define the desired
source ip's.
haproxy.router.openshift.io/whitelist:
192.168.1.10
haproxy.router.openshift.io/whitelist:
192.168.1.10 192.168.1.11 192.168.1.12
haproxy.router.openshift.io/whitelist:
192.168.1.0/24
haproxy.router.openshift.io/whitelist:
180.5.61.153 192.168.1.0/24 10.0.0.0/8
Trello: TbZPhHKE Route security management by end user
https://trello.com/c/TbZPhHKE/
Bug: 1426562
https://bugzilla.redhat.com/show_bug.cgi?id=1426562
This was ported from PR 13729
Committer: [email protected]
Author: [email protected]